fix(security): restrict /monitoring/events to configured administrators (Closes #101) - #163
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used🪛 dotenv-linter (4.0.0).env.example[warning] 54-54: [UnorderedKey] The PROOFDESK_ADMIN_LOGINS key should go before the PROOFDESK_DATA_DIR key (UnorderedKey) 🔇 Additional comments (4)
📝 WalkthroughWalkthroughAdds a configurable GitHub-login administrator allowlist and applies it to ChangesMonitoring event authorization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant requireAccessToken
participant requireAdmin
participant MonitoringEvents
Client->>requireAccessToken: Request GET /monitoring/events
requireAccessToken->>requireAdmin: Authenticated request with authSession
requireAdmin->>MonitoringEvents: Allow configured administrator login
MonitoringEvents-->>Client: Monitoring events response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ef4783d
into
harsharajkumar-273:main
Closes #101
Problem
GET /monitoring/eventsinbackend/src/routes/system.routes.tsis mounted withrequireAccessTokenand nothing else. That middleware only proves someone is authenticated — it performs no role or ownership check.The endpoint returns
readRecentMonitoringEvents(), which is the system-wide event log: backend and frontend stack traces, internal filesystem paths, request metadata and user agents for every user of the instance. Any authenticated caller — a regular contributor, a guest session, or anyone holding a bearer token — can read all of it.Change
backend/src/middleware/auth.ts— adds arequireAdminmiddleware backed by a newPROOFDESK_ADMIN_LOGINSvariable (comma-separated GitHub logins, matching the existingPROOFDESK_*convention).It denies in three cases, all deliberately fail-closed:
requireAccessTokenalso accepts a raw bearer token, and in that pathreq.authSessionis null — a bearer token on its own carries no verified identity to compare against the list;Logins are compared case-insensitively, matching GitHub's own treatment. The list is parsed per request so an operator can change it without a rebuild.
Identity is read from
req.authSession?.user?.login— the same primitive the existingcheckWorkspaceOwnermiddleware already uses, rather than introducing a second notion of identity.backend/src/routes/system.routes.ts—/monitoring/eventsnow runsrequireAccessToken, requireAdmin..env.example— documents the new variable.Compatibility
Nothing in the repository reads
/monitoring/events— the only occurrence of that path anywhere is the route definition itself. The frontend posts to/monitoring/client-errorbut never reads the event log back. So restricting it removes no existing functionality; an operator who wants access adds their login toPROOFDESK_ADMIN_LOGINS.Verification
npx tsc --noEmitinbackend/clean.requireAdminexercised across all seven paths: unset allow-list denies, non-admin denies, admin passes, case-mismatched login passes, bearer-token-without-session denies, session-without-login denies, whitespace-padded multi-entry list passes.Observation, not addressed here
GET /metrics(line 42 of the same file) has the same shape —requireAccessTokenwith no role check — and exposes the Prometheus register. I've left it alone since #101 names only/monitoring/events, but if you'd like it under the same guard, say so and I'll add it here or open it separately.Note:
npm testis red on cleanmainindependently of this branch.Summary by CodeRabbit